[ZEPPELIN-6630] Render the /configuration table through a React remote behind a flag - #5436
[ZEPPELIN-6630] Render the /configuration table through a React remote behind a flag#5436kimyenac wants to merge 2 commits into
Conversation
tbonelee
left a comment
tbonelee
left a comment
There was a problem hiding this comment.
Looks good overall. One thing: the safety-net spec's selector is pinned to the Angular implementation, so it will not survive the flip it exists to protect. Happy to merge once the comment below is addressed.
| constructor(page: Page) { | ||
| super(page); | ||
| this.pageDescription = page.locator('text=Shows current configurations for Zeppelin Server.'); | ||
| this.table = page.locator('zeppelin-configuration nz-table'); |
There was a problem hiding this comment.
Pinned to nz-table, this locator takes all six tests in this spec down the moment the flag defaults on or the Angular branch is removed. e2e/AGENTS.md also points at a shared data-testid that both implementations render at a seam.
If the first commit adds a neutral id to the Angular table and the second commit's React branch renders the same id, the commits stay independent and the spec survives the flip.
There was a problem hiding this comment.
Good catch, fixed in bced864.
The first commit now puts data-testid="configuration-table" on the Angular
table and the page model selects that, and the second commit's React branch
renders the same id. The flag spec no longer needs a branch-specific table id
either: both branches render configuration-table, and the mount host around it
is what tells them apart, so [data-testid="react-configuration-table"] [data-testid="configuration-table"]
scopes an assertion to the remote when it needs to be.
Checked the flip it is meant to survive: with configurationTable.defaultEnabled
flipped to true, so the page serves the React table by default, all seven tests
in the structure spec still pass. The 6363 commit also still stands alone, its
spec passes with none of the port applied.
The page had no spec of its own. What existed touched it only in passing, through header navigation and the user menu. Covers what the page promises: the header title and the security note in its description, the Name and Value table, and that every entry carries a name while a value may legitimately be empty, either unset or withheld as a secret. Two assertions go past structure and pin behaviour the component owns: the entries come out sorted by name, which is the sort in getAllConfig(), and they survive a reload. Removing that sort fails the spec. Reaching the page by direct URL is covered separately from the menu path, since /configuration has no route guard and is reachable either way. The table gets a data-testid rather than being matched as nz-table. This page is a migration seam, so a locator pinned to the ng-zorro element would take the whole spec down the moment the React table takes over, which is the flip these tests exist to protect.
…e behind a flag The page keeps its Angular shape: ConfigurationService still fetches and sorts the entries, zeppelin-page-header stays where it is, and routing is untouched. Only the table inside .content moves, and it moves as rendering alone, so the remote owns no state and this does not wait on the host/remote state-sharing decision. Both branches stay in the template behind ReactFeatureService, which ZEPPELIN-6564 introduced, under a new configurationTable surface reading ?reactConfiguration. The remote renders the same configuration-table id the Angular table carries, so specs that describe the page keep working on either side of the flag; the mount host around it is what tells the two branches apart. An onError from the remote falls back to the Angular table for the rest of the session. The props getter is memoized on configEntries: an object literal would hand ReactMountDirective a new identity on every change-detection pass. The remote renders antd's Table through ZeppelinThemeProvider, so it follows the shell's theme rather than the shell's stylesheets. Column headers are the one difference visible side by side, since antd draws them at font-weight 600 and ng-zorro at 500, so the surface passes fontWeightStrong through the provider's token prop. The rest is left at library defaults: measured against the Angular table, font size, cell padding and background match, and row height differs by 1px in light mode. jsdom implements no matchMedia and antd's responsive observer calls it while rendering, so the React test setup stubs it. Without that, Table throws and renders nothing under vitest.
1a59665 to
bced864
Compare
2 participants
What is this PR for?
Renders the
/configurationtable through the React remote behind a flag, and adds the e2e coverage the page was missing. ZEPPELIN-6630 recommends doing ZEPPELIN-6363 first or alongside, so both are here, as two commits: the safety net first, then the change it protects.[ZEPPELIN-6363]stands on its own, its spec passes with none of the port applied, so the two split cleanly if you would rather take them separately.ZEPPELIN-6363, first commit. The page had no spec of its own; what existed touched it only in passing through header navigation and the user menu. The new spec covers the header title and the security note, the Name and Value table, and that every entry carries a name while a value may legitimately be empty. Two assertions go past structure and pin behaviour the component owns: the entries come out sorted by name, which is the sort in
getAllConfig(), and they survive a reload. The page model deliberately selectszeppelin-configuration nz-tablerather than thedata-testidthe second commit introduces, so nothing about this commit depends on the port.ZEPPELIN-6630, second commit. The suggested boundary is kept:
ConfigurationServicestill fetches and sorts,zeppelin-page-headerstays where it is, routing is untouched, and only the table inside.contentmoves. The remote receives the sorted[string, string][]and owns no state, so this does not wait on the host/remote state-sharing decision.ReactFeatureServicefrom ZEPPELIN-6564, as a newconfigurationTablesurface reading?reactConfiguration. No new parsing.data-testideach, and anonErrorfrom the remote falls back to the Angular table for the rest of the session.configEntries, the only input that changes.shallowEqualsinparagraph.component.tsis a private helper of that component; rather than copy it, a single reference compare does the job here.queryParamMapis subscribed rather than read once, because navigating between/configurationand/configuration?reactConfigurationreuses the component. Replacing the subscription with a snapshot read fails the parity spec.On visual consistency: the remote renders antd's
TableinsideZeppelinThemeProvider, so it follows the shell's theme rather than the shell's stylesheets. Measured against the Angular table, font size, cell padding and background match in both themes. Column headers were the one difference visible side by side, since antd draws them at 600 and ng-zorro at 500, so the surface passesfontWeightStrongthrough the provider's token prop. Row height still differs by 1px in light mode, which is a library default and left alone.Two things worth raising for the surfaces that follow:
ReactMountDirective, which could skiphandle.update()when the incoming props are shallow-equal, would remove that from each call site. It felt out of scope here, so it is only a suggestion.src/pages/index.tsis not updated. Nothing imports that barrel, andexport *from a second page conflicts onmount, which is inherent to the mount-per-module contract.projects/zeppelin-react/src/test-setup.tsgains amatchMediastub: jsdom implements none and antd's responsive observer calls it while rendering, soTablethrows and the error boundary renders nothing. Any later spec touching an antd component would hit the same wall.What type of PR is it?
Improvement
Todos
None
What is the Jira issue?
How should this be tested?
e2e/tests/workspace/configuration/, 12 tests, no retries and no flakes.react-footer.spec.tsandpublished-paragraph.spec.tsre-run for regressions, 21 tests green.projects/zeppelin-reactat 43 including the newConfigurationTable.spec.tsx, andnpm run test:shellat 4.ConfigurationTableshows up inremoteEntry.js) andng build --configuration production.getAllConfig()fails the ZEPPELIN-6363 sort spec; replacing the query param subscription with a snapshot read fails the parity spec; removing thematchMediastub fails four vitest specs./#/configurationand/#/configuration?reactConfiguration=true.Firefox and WebKit were not run locally.
Screenshots (if appropriate)
The two tables side by side are hard to tell apart, which is the intent. In light mode the header weight now matches and only row height differs, by 1px.
Questions: